For now, only added it for TrivialB0Model to first see how this fits into what was expected in the issue.

A few notes:

• Following ENH: PET uptake model #112 (comment), almost all of this has been borrowed from scikit-learn. If we keep this approach, maybe a mention to this will need to be made.
• In scikit-learn:
  • Their base estimator does not have any is_fitted_ variable. It does not have a fit/predict either; they are left for child classes.
  • They rely on a list of attributes (coef_, estimator_) existing for the estimator and provided as a parameter https://github.com/scikit-learn/scikit-learn/blob/872124551/sklearn/utils/validation.py#L1551. If none is provided, the estimator is considered fitted if there exists an attribute that ends with an underscore and does not start with a double underscore (e.g. could be the is_fitted_ attribute).
• Their __sklearn_is_fitted__ is defined in child classes, and is used as an alternative to the above two possibilities to check whether an estimator is fit.
• For now, I've left the new modules in the eddymotion package, but they should probably be moved to an e.g. utils modules.

Useful references:
https://scikit-learn.org/stable/developers/develop.html#developer-api-for-check-is-fitted
https://scikit-learn.org/stable/auto_examples/developing_estimators/sklearn_is_fitted.html#sphx-glr-auto-examples-developing-estimators-sklearn-is-fitted-py

Slightly related:

• eddymotion models (e.g. TrivialB0Model, etc.) do not inherit from BaseModel. Not sure if that is intended, but it is counter-intuitive or the rationale would need to be documented better.
• Adds to the confusion having an EddyMotionEstimator class that has a fit but not a predict. Maybe model vs. estimator naming should be reworked.

No tests added for now until the above is figured out.

eddymotion models (e.g. TrivialB0Model, etc.) do not inherit from BaseModel. Not sure if that is intended, but it is counter-intuitive or the rationale would need to be documented better.

Should they inherit from BaseModel?

I very much agree with the suggestions. I also agree that fit() was a very poor choice for the estimator object. Probably optimize() or search() would have been much better names. +1 to updating.

Not sure if optimize() or search() are less confusing to me. If I read what its fit returns, compute_transform seems more descriptive and unambiguous wrt a fit/predict paradigm. To be addressed in a separate PR.

Concerning c440406, the test_model tests will not pass on CircleCI since the models do not inherit from BaseModel. If they are not supposed to inherit from it, I would need to relocate the __model_is_fitted__ method or give the attributes to the check_is_fitted calls in TrivialB0Model, and AverageDWModel.

Should they inherit from BaseModel?

I believe they do not inherit just for historical reasons (they were written the first, then we created BaseModel to provide a gateway for DIPY models and never did the last step of including them in the hierarchy). So, the answer is yes.

Concerning c440406, the test_model tests will not pass on CircleCI since the models do not inherit from BaseModel. If they are not supposed to inherit from it, I would need to relocate the __model_is_fitted__ method or give the attributes to the check_is_fitted calls in TrivialB0Model, and AverageDWModel.

Yup, I think making them part of the hierarchy makes all sense.

Not sure if optimize() or search() are less confusing to me. If I read what its fit returns, compute_transform seems more descriptive and unambiguous wrt a fit/predict paradigm. To be addressed in a separate PR.

Yes, in a different PR :). compute_transform feels a bit wordy and even confusing (in the sense that we are actually computing a lot of transforms). What about Estimator.estimate()?

Yes, in a different PR :). compute_transform feels a bit wordy and even confusing (in the sense that we are actually computing a lot of transforms). What about Estimator.estimate()?

PR #173.

Making this a draft as addressing the inheritance issue is more complicated that what it seems.